Conversation
🦋 Changeset detectedLatest commit: 5f7769a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds GCP KMS integration and runtime envs, replaces default keeper with a named Changes
Sequence DiagramsequenceDiagram
participant Client
participant PersonaHook as Persona Hook
participant RiskSvc as Risk Assessment
participant Allower as Firewall/Allower
participant GCP as GCP KMS
participant Chain as Chain / keeper
Client->>PersonaHook: POST inquiry
PersonaHook->>RiskSvc: perform risk assessment
RiskSvc-->>PersonaHook: pass/fail
alt pass
PersonaHook->>PersonaHook: derive account
PersonaHook->>Allower: getAllower()
Allower->>GCP: init credentials / request KMS-backed account
GCP-->>Allower: LocalAccount / credentials
Allower-->>PersonaHook: allower client ready
PersonaHook->>Allower: allow(account)
Allower-->>PersonaHook: allow result
PersonaHook->>Chain: keeper.poke(account, { ignore: [...] })
Chain-->>PersonaHook: tx hash / result
PersonaHook-->>Client: 200 success
else fail
PersonaHook-->>Client: rejection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @aguxez, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the server's account management and security by integrating Google Cloud KMS for a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #839 +/- ##
==========================================
- Coverage 71.23% 71.10% -0.13%
==========================================
Files 212 213 +1
Lines 8378 8525 +147
Branches 2741 2783 +42
==========================================
+ Hits 5968 6062 +94
- Misses 2132 2165 +33
- Partials 278 298 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c642d46 to
88aa6aa
Compare
Additional Comments (1)
At runtime this is safe — only ETH entries have Consider asserting non-null at the call site to make the invariant explicit: Or narrow the type at build time by storing ETH entries and market entries in separate arrays. |
| await keeper | ||
| .poke(account, { ignore: [`NotAllowed(${account})`] }) | ||
| .catch((error: unknown) => captureException(error, { level: "error" })); |
There was a problem hiding this comment.
🚩 NoBalance() special handling removed without replacement
The old activity.ts had a specific retry-and-capture flow for NoBalance() errors: it used { ignore: ["NoBalance()"] } in exaSend, then re-threw inside .then() to trigger withRetry, and finally captured the error as a warning (not error) if all retries were exhausted.
The new code passes { ignore: [\NotAllowed(${account})`] }instead, meaningNoBalance()is no longer ignored byexaSend. If a poke reverts with NoBalance(), the withRetryinsidepoke (server/utils/accounts.ts:137-163) will retry it (default retry behavior), but each retry's failure is captured at errorlevel viaexaSend's catch block — previously it was only warning` after all retries.
The corresponding test (captures no balance once after retries) was also removed. This seems intentional per the PR's simplification goals, but changes error noise characteristics in Sentry.
Was this helpful? React with 👍 or 👎 to provide feedback.
17aa7dc to
115a36a
Compare
Additional Comments (2)
Downstream in await initializeGcpCredentials(); // returns cached no-op promise
if (!(await hasCredentials())) throw … // file is gone → always throwsThe server cannot recover without a full restart. The Suggested fix: Call export async function getAccount(): Promise<LocalAccount> {
await initializeGcpCredentials();
if (!(await hasCredentials())) {
resetGcpInitialization(); // allow next caller to retry
throw new Error(…);
}
…
}
These GCP env-var checks run at module import time, before any GCP-dependent code is invoked. This breaks the deployment of unrelated features if GCP credentials are not configured, and makes testing hooks that use Suggested fix: Move the GCP env-var validation inside export async function getAccount(): Promise<LocalAccount> {
if (!process.env.GCP_PROJECT_ID) throw new Error("GCP_PROJECT_ID is required");
const projectId = process.env.GCP_PROJECT_ID;
if (!/^[a-z][a-z0-9-]{4,28}[a-z0-9]$/.test(projectId)) {
throw new Error("GCP_PROJECT_ID must be a valid GCP project ID format");
}
// … validate GCP_KMS_KEY_RING and GCP_KMS_KEY_VERSION here too
await initializeGcpCredentials();
…
}This keeps |
Additional Comments (3)
Prefer the explicit exponentiation operator instead:
The triple base64 decode ( Consider adding a comment to the constant (and possibly the app.yaml instructions) documenting the expected encoding:
The |
a4d043e to
e5dca00
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5dca00480
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| keeper | ||
| .poke(account, { | ||
| notification: { | ||
| headings: { en: "Account assets updated" }, | ||
| contents: { en: "Your funds are ready to use" }, | ||
| }, | ||
| }) | ||
| .catch((error: unknown) => captureException(error, { level: "error" })); |
There was a problem hiding this comment.
Move poke after user creation succeeds
keeper.poke is triggered before createUser and before persisting pandaId, so transient onboarding failures can cause Persona webhook retries to run poke repeatedly for the same inquiry. Because this call may submit on-chain pokes and sends the "Account assets updated" notification, retries can produce duplicate transactions/notifications even though onboarding did not complete; this side effect should run only after successful user creation is persisted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f7769a467
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (result.status === "rejected") { | ||
| captureException(result.reason, { level: "error" }); | ||
| return []; |
There was a problem hiding this comment.
Do not drop rejected balance reads in poke
When a getBalance/balanceOf call fails, this branch captures the exception and returns [], which silently removes that asset from assetsToPoke. In the presence of transient RPC failures, a funded market can be skipped while poke still resolves as success, so callers have no signal to retry and the account can miss activation/yield on that asset until a later unrelated trigger.
Useful? React with 👍 / 👎.
| let json = gcpBase64Json; | ||
| for (let index = 0; index < DECODING_ITERATIONS; index++) { | ||
| json = Buffer.from(json, "base64").toString("utf8"); | ||
| } | ||
| await writeFile(GOOGLE_APPLICATION_CREDENTIALS, json, { mode: 0o600 }); |
There was a problem hiding this comment.
🔴 GCP credentials written to filesystem violates "secrets never in files" rule
AGENTS.md/CLAUDE.md explicitly states: "production secrets are environment variables at runtime only - never in files." The new gcp.ts decodes GCP_BASE64_JSON and writes the GCP service account JSON to /tmp/gcp-service-account.json, which is then used by KeyManagementServiceClient via keyFilename in server/utils/accounts.ts:361-363. The @google-cloud/kms KeyManagementServiceClient supports a credentials option that accepts a parsed JSON object directly in memory, eliminating the need to write secrets to the filesystem. This would avoid both the rule violation and the security risk of persisting credentials on disk.
Prompt for agents
In server/utils/gcp.ts, instead of writing the decoded GCP credentials JSON to a file at /tmp/gcp-service-account.json, export the parsed credentials object directly. Replace the file-write logic (lines 21-33) with a function that returns the parsed JSON object. Then in server/utils/accounts.ts lines 361-363, replace `keyFilename: GOOGLE_APPLICATION_CREDENTIALS` with `credentials: getGcpCredentials()` (passing the parsed JSON object directly to the KeyManagementServiceClient constructor). This avoids writing production secrets to the filesystem, which is explicitly prohibited by the project rules. The @google-cloud/kms KeyManagementServiceClient constructor accepts a `credentials` option that takes { client_email, private_key } directly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const account = await withRetry( | ||
| () => | ||
| gcpHsmToAccount({ | ||
| hsmKeyVersion: `projects/${projectId}/locations/us-west2/keyRings/${keyRing}/cryptoKeys/allower/cryptoKeyVersions/${version}`, |
There was a problem hiding this comment.
🟡 GCP KMS location us-west2 hardcoded while other KMS path components are parameterized
In the KMS key version path at server/utils/accounts.ts:360, the GCP location is hardcoded as us-west2 while project ID, key ring, and version are all configurable via environment variables (GCP_PROJECT_ID, GCP_KMS_KEY_RING, GCP_KMS_KEY_VERSION). If the KMS key ring is provisioned in a different region (e.g., for a staging environment or disaster recovery), the code will fail to find the key and must be changed instead of just updating an environment variable. This inconsistency makes the configuration fragile.
Prompt for agents
In server/utils/accounts.ts, add a new environment variable (e.g., GCP_KMS_LOCATION) alongside the existing GCP_PROJECT_ID, GCP_KMS_KEY_RING, and GCP_KMS_KEY_VERSION checks at the top of the file (~line 53-63). Then use it in the hsmKeyVersion template string at line 360 to replace the hardcoded 'us-west2'. Also add the new env var to .do/app.yaml, server/vitest.config.mts, and server/script/openapi.ts for consistency with the other GCP config values.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async onFetchRequest(request) { | ||
| const clone = request.clone(); | ||
| captureRequests(parse(Requests, await clone.json())); | ||
| }, |
There was a problem hiding this comment.
🚩 onFetchRequest now clones request before reading body — inconsistent with publicClient.ts
The new keeper transport at server/utils/accounts.ts:69-72 clones the request before calling .json(): const clone = request.clone(); captureRequests(parse(Requests, await clone.json()));. The old keeper.ts consumed the body directly without cloning (matching publicClient.ts:15-17). The clone is safer since calling .json() consumes the request body per the Fetch API spec, which could interfere with the transport's actual HTTP call. However, publicClient.ts still uses the old non-cloning pattern. The allower transport at server/utils/accounts.ts:385-393 also clones and adds a try-catch wrapper. The inconsistency between the keeper/allower transports (clone) and publicClient (no clone) should be investigated.
Was this helpful? React with 👍 or 👎 to provide feedback.
| import "../mocks/accounts"; | ||
| import "../mocks/auth"; | ||
| import "../mocks/deployments"; | ||
| import "../mocks/keeper"; | ||
| import "../mocks/onesignal"; | ||
| import "../mocks/pax"; | ||
| import "../mocks/persona"; |
There was a problem hiding this comment.
🚩 onesignal mock removed from card.test.ts
card.test.ts previously imported ../mocks/onesignal which mocked sendPushNotification to a no-op. This import was removed in the PR. The card API at server/api/card.ts:33,383 imports and calls sendPushNotification. If any test path in card.test.ts triggers a notification (e.g., card mode changes), the real onesignal module would be used. Since the test env has no ONESIGNAL_API_KEY, the call might fail silently (caught by .catch()) or cause unexpected behavior. The tests presumably still pass in CI, suggesting notification paths aren't triggered in the current test cases, but this removal reduces test isolation.
Was this helpful? React with 👍 or 👎 to provide feedback.
closes #643
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Greptile Summary
This PR integrates Google Cloud KMS (via
@google-cloud/kmsand@valora/viem-account-hsm-gcp) to sign firewallallowtransactions with an HSM-backed key, and adds a newkeeper.pokeutility that proactively pokes account assets (ETH, WETH, ERC-20) after KYC approval and on activity webhooks.Key changes:
server/utils/gcp.ts— new module handles credential bootstrapping (triple base64 decode →/tmp/gcp-service-account.json), lazy initialization with promise caching, and retryable KMS error classification.server/utils/accounts.ts— addsgetAccount()(GCP KMS → viemLocalAccount),allower()(wallet client that wrapsallowon the Firewall contract), andpoke()(scans balances and pokes every non-zero asset viaexaSend).server/hooks/persona.ts— after KYC approval callsallowthen fire-and-forgetspoke;allowerPromisecaches the expensive KMS init across requests.server/hooks/activity.ts— callspokeafter account deployment, withNotAllowedin the ignore list.Verified findings:
delayfunction inpoke'swithRetryuses bitwise left-shift which overflows atcount >= 31(currently safe withretryCount: 10, but a footgun if the parameter is raised).DECODING_ITERATIONS = 3constant lacks documentation explaining the triple base64 encoding strategy.keeperclient'sonFetchRequesthook lacks error handling, unlike theallowerversion, making it susceptible to unhandled exceptions on malformed RPC payloads.Confidence Score: 3/5
retryCountis raised, (2) the triple base64 decoding strategy lacks documentation creating fragility, and (3) the missing try/catch inkeeper'sonFetchRequestcreates an inconsistency withallowerthat could surface unhandled exceptions. None of these are critical under normal conditions, but they represent code-quality debt worth fixing before production ramp-up.server/utils/accounts.ts(bitwise-shift delay, error handling inconsistency) andserver/utils/gcp.ts(undocumented constant).Last reviewed commit: 0275499
Context used:
dashboard- AGENTS.md (source)dashboard- CLAUDE.md (source)This is part 1 of 2 in a stack made with GitButler: